-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Melihat subscription package #71
Conversation
src/redux/api/subscriptionApi.ts
Outdated
providesTags: (result) => [ | ||
{ type: 'LatestSubscription', id: result?.id }, | ||
'LatestSubscription', | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the documentation, it's always a great idea to check first whether result variable exist or not. You can refer it here:
https://redux-toolkit.js.org/rtk-query/usage/automated-refetching#providing-cache-data
src/app/package/page.tsx
Outdated
const { data: free_package = {} as PackageDetailResponse } = | ||
useGetPackageDetailQuery(1) | ||
const { data: premium_package = {} as PackageDetailResponse } = | ||
useGetPackageDetailQuery(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, you can use variable for the number. I get what you are saying because 1 means package with id: 1. But what if the schema changes and id become UUID format, this will be hard to maintain, so you can refactor this to a constant.ts so that you don't have to change the main package list code.
constant.ts
export const PACKAGE = {
"free": 1,
"premium": 2
}
Then you can call these constant, such as PACKAGE["free"]
Feels free to have an opinion about my comments though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have fixed! Please check 😃 @emiriko @JohannesSetiawan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good implementation, but please address the problem that alvaro mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request introduces several updates to improve the user experience and provide more information to our users.
Changes Made: